unwind safety fixes by Mirko-A · Pull Request #140 · Rust-for-Linux/pin-init

Mirko-A · 2026-04-27T18:48:36Z

Adds unwind-safety fixes to in-place initialization helpers so partially initialized values are dropped on both errors and panics. Changes:

Add __internal::ArrayInit guard type that safely initializes an array by running an initializer on each element, keeping track of the number of initialized elements. When dropped, if the initialization was partially successful, the guard drops the initialized portion of the array. Use the guard in [pin_]init_array_from_fn.
Use __internal::DropGuard in [pin_]chain so a panic/error in the chained closure drops the value initialized in the first stage; mem::forget the guard on success.
Add regression tests.

Closes: #136

The approach taken here was suggested in the issue itself, thus I added a Suggested-by tag. I hope that is okay.

Edits:
v1 -> v2:

rename ArrayInitGuard to ArrayInit
implement [Pin]Init for ArrayInit instead of wrapping it in [pin_]init_from_closure

v2 -> v3:

do not mem::forget the ArrayInit guard on success; state that the guard will only drop initialized elements on partial success

nbdd0121 · 2026-05-04T14:52:30Z

        let val = unsafe { Pin::new_unchecked(val) };
-        // SAFETY: `slot` was initialized above.
-        (self.1)(val).inspect_err(|_| unsafe { core::ptr::drop_in_place(slot) })
+        (self.1)(val)?;


The whole implementation here can probably be simplified to something like

(self.1)(unsafe { Slot::new(slot) }.init(self.0).let_binding()) or something similar if #143 lands.

I could rebase this branch to dev/accessor-rework if you'd like? Otherwise I don't mind waiting for #143 to land.

nbdd0121 · 2026-05-05T13:43:19Z

I cannot review diff of diffs. Please always squash your changes to individually meaningful commits.

Mirko-A · 2026-05-05T13:53:23Z

I see, everything has been squashed to the original three commits.

When reviewing, I prefer having diffs that do exactly what each of the comments suggests and squashing after the review. I will squash future fixups immediately.

nbdd0121 · 2026-05-05T14:04:05Z

For GitHub-centric workflow that's indeed convenient, but for this project as the commits are synced to the kernel I need to review each commit individually so it's up to kernel's quality requirement.

Honestly the actual issue is that GitHub does not provide a good way to visualize the diff, despite that it has all the needed information to render as such. The "Compare" button sometimes does the job, but it breaks when you rebase and fixup commits in a single push. Longer term, either we can wait until GitHub implements it (which is quite unlikely given the slop), or we can adopt what the Rust Project's triagebot does by automatically generates a range diff like this.

Mirko-A · 2026-05-05T14:08:22Z

I understand and fully agree. Unfortunately, looking at recent trends at GitHub, I don't expect much in terms of improvements to the platform.

Thank you for taking the time to explain your reasoning. :)

nbdd0121

Thanks for working on this. Some further feedbacks.

Also, could you move the ArrayInit to lib.rs just before init_array_from_fn? The __internal module is supposed to only host items that need to be public due to the use of generated code from pin-init-internal (yes, we have a few pub(crate) there, but that's for historical reasons and I'm working on moving them).

nbdd0121 · 2026-05-09T11:50:54Z

+        // INVARIANT: `self.ptr` is non-null once initialization starts.
+        self.ptr = slot.cast::<T>();
+        for i in 0..N {
+            self.num_init = i;


Thanks for adding the invariant comments! You probably want to add a line here as well because num_init is also involved in the invariant (i.e. you need to assert that [..num_init] has been initialized already).

Thanks for adding the invariant comments!

You're welcome!

You probably want to add a line here as well...

Thanks to your other comment explaining the logic behind INVARIANT comments, this is very clear. I've added them both here and in __pin_init.

nbdd0121 · 2026-05-09T11:52:41Z

+        for i in 0..N {
+            self.num_init = i;
+            let init = (self.make_init)(i);
+            // SAFETY: Since `0 <= i < N`, `self.ptr.add(i)` is in array bounds and


The comparison chain is not valid Rust. I think you can probably just remove this part. Valid for write is also not a requirement of add.

Suggested change

// SAFETY: Since `0 <= i < N`, `self.ptr.add(i)` is in array bounds and

// SAFETY: `self.ptr.add(i)` is in bounds.

Understood, thanks.

nbdd0121 · 2026-05-09T11:54:57Z

+impl<T, F> Drop for ArrayInit<T, F> {
+    fn drop(&mut self) {
+        if self.ptr.is_null() {
+            // INVARIANT: Since `self.ptr` is null - either no initialization has


INVARIANT comment is needed when the fields involved in the invariant is changed. Simply relying on the invariant does not need special annotation (it's actually confusing to have them).

That makes a lot of sense, thank you.

nbdd0121 · 2026-05-09T11:56:59Z

+        // INVARIANT: Since `self.ptr` is not null - the initialization has failed
+        // partway and we need to drop elements initialized thus far to uphold the
+        // pinning requirement.
+        //
+        // SAFETY: Invariants of `ArrayInit` guarantee that, when `self.ptr` isn't
+        // null, elements `self.ptr[0..self.num_init]` are initialized and contain
+        // valid `T` values, so dropping them is safe.


Same here

Suggested change

// INVARIANT: Since `self.ptr` is not null - the initialization has failed

// partway and we need to drop elements initialized thus far to uphold the

// pinning requirement.

//

// SAFETY: Invariants of `ArrayInit` guarantee that, when `self.ptr` isn't

// null, elements `self.ptr[0..self.num_init]` are initialized and contain

// valid `T` values, so dropping them is safe.

// SAFETY: Since `self.ptr` is not null - the initialization has failed

// partway. Drop `self.ptr[0..self.num_init]` which are initialized per type invariant.

nbdd0121 · 2026-05-09T11:58:18Z

+        // null, elements `self.ptr[0..self.num_init]` are initialized and contain
+        // valid `T` values, so dropping them is safe.
+        unsafe {
+            let slice = core::ptr::slice_from_raw_parts_mut(self.ptr, self.num_init);


This should either be lifted out of the unsafe block, or just inlined into the drop_in_place (I actually prefer the latter).

Fixed (by inlining into drop_in_place ).

When arguments are produced by function-calls, I tend to bind them to variables - I find it easier to read that way (at the risk of being more verbose). But it absolutely shouldn't have been inside an unrelated unsafe block. Thank you for pointing it out.

Adds a guard type that safely initializes an array by running an initializer on each element, keeping track of the number of initialized elements. In the case of a panic or error in the per-element initializer, the guard drops the already-initialized portion of the array; nothing is dropped on success. The previous code only ran cleanup on the explicit error path. If the per- element initializer panicked partway through, the elements already written into the array would be leaked: their `Drop` impls would never run. Link: Rust-for-Linux#136 Reported-by: Gary Guo <gary@garyguo.net> Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

Adds a drop guard before the call to the chained closure so that the value initialized by the first stage is dropped if the closure errors or panics; `mem::forget` the guard on success. The previous code only ran cleanup on the explicit error path, leaking the first-stage value if the chained closure panicked. Link: Rust-for-Linux#136 Reported-by: Gary Guo <gary@garyguo.net> Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

Cover both fixes added in the series: - `[pin_]init_array_from_fn`: a panic or error from element `i`'s initializer drops the previously initialized elements `0..i`. - `[pin_]chain`: a panic or error from the chained closure drops the value initialized by the first stage. Also assert no double-drop on the success paths. Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

Mirko-A · 2026-05-12T19:53:25Z

Apologies for the delay.

Thanks for working on this.

Thank you! I really appreciate the level of detail in your review/feedback. It is rare to come by these days and I'm really glad to work with people that care this much about the code they maintain.

could you move the ArrayInit to lib.rs

Done. Also thank you for the clarification on what should go where, I tried to guess based on types like __internal::InitClosure.

I will respond to individual comments for the rest of your feedback.

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from 235fcfb to 45e29c0 Compare April 27, 2026 19:38

Mirko-A marked this pull request as ready for review April 27, 2026 20:00

nbdd0121 added the soundness Related to soundness issues label Apr 28, 2026

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch 2 times, most recently from 7ad7985 to 225f13b Compare May 1, 2026 23:11

nbdd0121 reviewed May 2, 2026

View reviewed changes

Comment thread src/__internal.rs Outdated

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e45f41e to e3b2cb6 Compare May 3, 2026 12:45

Mirko-A commented May 3, 2026

View reviewed changes

Comment thread src/__internal.rs Outdated

nbdd0121 reviewed May 4, 2026

View reviewed changes

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e3b2cb6 to e5f75a9 Compare May 5, 2026 13:35

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e5f75a9 to 5c83836 Compare May 5, 2026 13:49

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from 5c83836 to eebe948 Compare May 5, 2026 14:11

nbdd0121 reviewed May 7, 2026

View reviewed changes

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from eebe948 to 8b46ef9 Compare May 9, 2026 09:24

nbdd0121 reviewed May 9, 2026

View reviewed changes

Mirko-A added 3 commits May 12, 2026 21:16

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from 8b46ef9 to 59950ff Compare May 12, 2026 19:52

	// SAFETY: Since `0 <= i < N`, `self.ptr.add(i)` is in array bounds and
	// SAFETY: `self.ptr.add(i)` is in bounds.

Conversation

Mirko-A commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nbdd0121 commented May 5, 2026

Uh oh!

Mirko-A commented May 5, 2026

Uh oh!

nbdd0121 commented May 5, 2026

Uh oh!

Mirko-A commented May 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nbdd0121 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mirko-A commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Mirko-A commented Apr 27, 2026 •

edited

Loading